sequenceDiagram
  participant Client
  participant Adapter as RedisAdapter
  participant Coordinator
  participant FSM
  participant Store
  Client->>Adapter: RPUSH attempt 1
  Adapter->>Coordinator: Dispatch(startTS)
  Coordinator->>FSM: One-phase request (commitTS=T1, prevCommitTS=0)
  FSM->>Store: Apply mutations -> commit at T1
  FSM-->>Coordinator: Committed
  Adapter-->>Client: Retry signalled
  Adapter->>Adapter: Capture write set from attempt 1
  Adapter->>Coordinator: Dispatch(retry, prevCommitTS=T1)
  Coordinator->>FSM: One-phase request (commitTS=T2, prevCommitTS=T1)
  FSM->>Store: CommittedVersionAt(primaryKey, T1)?
  alt Found
    FSM-->>Coordinator: no-op (nil)
    Adapter->>Adapter: Return stored client-visible length
    Adapter-->>Client: Result (no duplicate)
  else Not found
    FSM->>Store: Apply mutations at T2
    FSM-->>Coordinator: Committed
    Adapter-->>Client: Result (applied)
  end

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude review

Claude finished @bootjp's task in 3m 40s —— View job

Design Review: Transaction Secondary-Commit Idempotency

• Load tools
• Read design document + relevant code
• Analyze design for correctness, completeness, and risks
• Post review feedback

Summary

The root cause analysis is accurate and the proposed server-side marker is the right enforcement layer. The design is sound for the primary-commit case. The issues below range from a hard correctness gap in M3 to a missing backward-compatibility section, with several areas worth clarifying before implementation starts.

Critical

1. ErrTxnAlreadyCommittedAtDifferentTS must carry commit_ts — M3 cannot reconstruct without it

M3's adapter recovery reads txnCommitKey(primary_key, start_ts) to find the actual commit_ts. But at that point start_ts is the current retry's regenerated startTS — not the original attempt's startTS that was recorded in the primary's commit record. The original startTS is gone because runTransaction regenerates it on each iteration.

The FSM already has the committed commit_ts in the marker (__txn_applied__<id> → (commit_ts, applied_at)). The error sentinel should carry it directly:

type ErrTxnAlreadyCommittedAtDifferentTS struct {
    RecordedCommitTS uint64
}

The adapter can then go directly to the actual state via resolveListMeta at RecordedCommitTS, bypassing the txnCommitKey lookup entirely. Without this, M3's result reconstruction has no reliable anchor. (Gemini flagged this too — worth being explicit that the fix is on the FSM side, not the error-type naming.)

Fix this →

2. Rolling-upgrade compatibility: zero/absent adapter_txn_id case is unspecified

During a rolling upgrade, nodes on old code will send AdapterTxnId = "" (proto default). The FSM on new-code nodes must skip the marker check when the field is absent/zero — otherwise it would write a marker keyed by the zero UUID, and the first zero-UUID request would block all subsequent zero-UUID requests from older nodes with "already committed."

This must be explicit in M1/M2: if AdapterTxnId is zero/empty, bypass both the marker read and the marker write. The marker only engages once the adapter has actually started sending UUIDs.

Fix this →

Important

3. Multi-shard retry safety — the design is correct but the reasoning needs to be made explicit

The caller audit doesn't explain WHY a Attempt 2 can't partially commit on secondary shards before the primary check fires. The safety actually flows from the coordinator structure: commitPrimaryTxn runs before commitSecondaryTxns. If commitPrimaryTxn returns ErrTxnAlreadyCommittedAtDifferentTS (because the primary shard already has the marker), commitSecondaryTxns is never called. Attempt 2's mutations therefore never reach secondary shards.

This is the key invariant that makes the multi-shard case safe. It's worth one sentence in the design explicitly linking the marker check to the primary-first commit ordering.

4. FSM hot-path read overhead missing from risks

R2 only calls out the 16-byte proto field. Every commit now also requires a Pebble point read (__txn_applied__<id>) on the FSM apply path. Under normal operation (no retries) this read always misses but still hits the Pebble read path. At 1000 txn/s, this is 1000 extra point reads/s through the FSM pipeline, which is synchronous and blocks the apply loop.

This is probably acceptable (Pebble point reads from block cache are sub-microsecond), but it should be in the risks section so reviewers of M2 can benchmark against it and decide if a bloom filter or skip-list layer helps.

5. GC — replace prefix scan with DeleteRange via UUIDv7 cutoff (addresses Gemini's medium finding)

The current M4 description calls for a prefix scan that reads every marker value to check applied_at_wall_time. Since adapter_txn_id is UUIDv7, its top 48 bits are a millisecond timestamp. The compactor can compute the UUIDv7 value corresponding to now - TTL and issue a single DeleteRange(__txn_applied__<min_uuid>, __txn_applied__<cutoff_uuid>). This avoids the O(N) read overhead entirely.

The UUIDv7 cutoff calculation is straightforward: shift the cutoff epoch-ms into bits [127:80] of a 128-bit value, zero the rest. Any UUIDv7 with a timestamp before the cutoff will compare less than this value under lexicographic byte order.

6. M5 availability risk — needs an explicit mitigation (addresses Gemini's high-priority finding)

The synchronous secondary-marker read in M5 runs on the already-degraded retry path. If the secondary shard is mid-leader-election, this read adds unbounded latency before the retry loop can proceed. Given that M5 is labeled "optional" the risk is bounded, but the doc should note: when implementing M5, gate the marker read on a short deadline (e.g. 200ms) so a slow shard read degrades to "skip the optimisation, let the FSM marker catch it" rather than blocking the retry indefinitely.

Minor

7. Caller audit — LockResolver row needs a clarifying split

The current row conflates two LockResolver sub-cases:

Both paths are handled correctly by the design, but spelling them out prevents the "same (start_ts, commit_ts)" reasoning from looking circular to implementers who will touch kv/lock_resolver.go.

8. TTL lower bound — check against gRPC deadline

The TTL formula 2 × (lockResolverInterval + defaultTxnLockTTLms) = 80s assumes the retry window is bounded by the LockResolver cycle. But redisDispatchTimeout (the per-Redis-command context deadline) is the tighter bound for retryRedisWrite. If redisDispatchTimeout > 80s, a retry can arrive after the marker is GC'd. The doc should verify (and state) that redisDispatchTimeout < TTL as a precondition, or derive the TTL from redisDispatchTimeout directly. If redisDispatchTimeout is configurable, add a startup assertion that markerTTL > redisDispatchTimeout.

Strengths worth noting

• The root cause walkthrough — tracing the exact key paths through listItemKey and ListMetaDeltaKey — is exactly the level of specificity the implementer will need. Strongly prefer this over hand-waving.
• Explicitly ruling out retryRedisWrite removal (§"Why not just remove...") is the right call and the explanation about OCC contention + Jepsen :fail classification is correct.
• Writing the marker atomically in the same Pebble batch as the mutations is the correct correctness requirement — this is the single sentence in the design that bears the most implementation weight.
• The UUIDv7 choice is apt: GC by time prefix, no coordination needed to issue IDs, globally unique for the retry-window lifetime.

Overall: approve the design direction, suggest addressing issues 1 and 2 before merging (M3 is blocked on 1; rolling upgrades are blocked on 2), and fold 5 and 8 into M4's scope.

@claude review

Round-2 fixes addressing codex P1 + CodeRabbit Major (both flagged the same legacy-Coordinate caller-audit gap) and the two reviewdog cyclop warnings:

P1 (codex) / Major (CodeRabbit) — onePhaseTxnRequest legacy callers dropped PrevCommitTS:

• Coordinate.dispatchTxn (kv/coordinator.go:465) now takes a prevCommitTS param and forwards it via onePhaseTxnRequestWithPrevCommit. Production call site passes reqs.PrevCommitTS; the 5 unit-test callers pass 0.
• buildRedirectRequests (kv/coordinator.go:957) now forwards reqs.PrevCommitTS unconditionally — it is the adapter-allocated identity of the prior attempt and is never re-issued by the leader (unlike StartTS/CommitTS, which the redirect path clears when StartTS==0).
• Caller audit: every onePhaseTxnRequest site in production now routes through the prev-commit-aware path. The legacy onePhaseTxnRequest(...) is kept only as a 0-prev wrapper for backwards source compat.

Cyclop (reviewdog):

• decodeTxnMetaV2 12 → ~6 by table-driving the V2 optional fields through optionalV2Fields().
• adapter test Dispatch 12 → ~7 by extracting the probe to maybeProbe().

Doc nits (CodeRabbit):

• Status header now "partial — option 2 reader landed everywhere; writer (emission) gated off by default," resolving the contradiction with the landed-milestone notes below.
• All bare code fences carry a language tag (text/go).

Commit: fd0831cd. kv/store suites green; gofmt + vet clean.

Claude finished @bootjp's task in 3m 21s —— View job

Round-2 Code Review

• Load tools
• Read changed files: coordinator.go, txn_codec.go, redis_list_dedup_test.go, design doc
• Review PrevCommitTS threading fix
• Review cyclop complexity fixes
• Review doc updates
• Post final findings

Verdict: Round-2 changes are correct and the targeted issues are resolved.

The P1 (codex) / Major (CodeRabbit) gap — legacy Coordinate path silently dropping PrevCommitTS — is fixed correctly at both sites. Cyclop targets are addressed. One minor dead-code note and one comment nit follow.

PrevCommitTS Threading (kv/coordinator.go)

Both callsites are correct:

• dispatchTxn (line 801): New prevCommitTS uint64 parameter is wired from reqs.PrevCommitTS at the callsite (line 465) and passed through to onePhaseTxnRequestWithPrevCommit. ✅
• buildRedirectRequests (line 987): Forwards reqs.PrevCommitTS unconditionally. The StartTS == 0 → commitTS = 0 reset correctly doesn't touch PrevCommitTS, since the comment accurately captures the invariant: the prior attempt's identity is adapter-allocated and must not be discarded when the leader regenerates timestamps. ✅

Minor: onePhaseTxnRequest is now dead code. All production callers now go through onePhaseTxnRequestWithPrevCommit directly. The test callers (coordinator_txn_test.go) were updated to pass ..., 0 to dispatchTxn, not to call onePhaseTxnRequest. The function has zero callsites. golangci config doesn't include deadcode/unused, so this won't fail lint — but it's a semantic footgun. The safe options:

• Delete it (cleanest, no risk because there are truly no callers), or
• Keep it but add a comment: // zero-prevCommitTS wrapper; prefer onePhaseTxnRequestWithPrevCommit.

Fix this →

Missing test: buildRedirectRequests PrevCommitTS forwarding is code-correct but has no unit coverage. There are no tests calling buildRedirectRequests with a non-zero PrevCommitTS to verify the field survives the leader-redirect path. Given this was the exact defect that prior reviews flagged, a targeted test would lock it down.

Cyclop Fix: optionalV2Fields (kv/txn_codec.go)

The approach is correct. Field order in the table matches the encoder order (LockTTL → CommitTS → PrevCommitTS) and the decode loop reads them identically. Round-trip is preserved.

Minor: the comment on optionalV2Fields (line 201–205) is misleading. It says "encodeTxnMetaV2 and decodeTxnMetaV2 must traverse this list in the same sequence" — but encodeTxnMetaV2 doesn't use the table at all; it has its own manually-ordered conditionals. The comment creates a false impression that both encoder and decoder are driven by the table (which would be a nice property but isn't true). Should read something like: "decodeTxnMetaV2 reads fields via this table; encodeTxnMetaV2 must write them in the same order — update both together when adding a field."

Fix this →

Cyclop Fix: maybeProbe extraction (adapter/redis_list_dedup_test.go)

The extraction is clean and the (handled bool, resp, err) triple-return idiom is idiomatic Go for probe-owns-result patterns. ✅

Design Doc

Status header is accurate (partial — option 2 reader landed everywhere; writer (emission) gated off by default) and the bare code fences now carry text tags. Both CodeRabbit nits resolved. ✅

Carry-over from Round-1 (not in scope for Round-2, but not yet addressed)

For visibility — these were not in Round-2's stated scope but are still open:

1. CommittedVersionAt retention guard (codex P2 on store/lsm_store.go:778, store/mvcc_store.go:476): Both implementations return (false, nil) when the probe timestamp is below minRetainedTS, making a compacted-history miss indistinguishable from "never landed." Should return ErrReadTSCompacted to allow the FSM/adapter to distinguish "not found" from "history gone." Low urgency while compaction windows are long relative to retry windows, but worth tracking.

2. TestListPushDedup_DisabledKeepsLegacyPath (CodeRabbit nitpick): Still uses ambiguousDispatch=0, so the legacy path is tested only for the no-retry case. Not a blocking issue.

Summary: The Round-2 changes correctly resolve the P1/Major gap. Two cosmetic issues (dead onePhaseTxnRequest wrapper, misleading optionalV2Fields comment) and one missing unit test for the redirect path are the only remaining items before merge.

@claude review

Round-3 fixes addressing codex P1 (adapter/redis.go:3246, 06:37 post-fd0831cd) + the golangci unused lint fail on the dead onePhaseTxnRequest wrapper + the misleading optionalV2Fields comment.

codex P1 — reuse-after-meta-change can hide writes (adapter/redis.go):
Captured ops were replayed against whatever the world had become; an intervening RPOP/LPOP/LTRIM that shrank the boundary could leave the reused seq past the new Tail (physically written, unreachable to LRANGE).

Fix: capture the boundary positions (listItemKey(Head), listItemKey(Tail-1)) at attempt 1's meta read and pass them through OperationGroup.ReadKeys. ApplyMutationsRaft validates them atomically against startTS under applyMu, so any boundary-touching commit fires WriteConflict; listPushCoreWithDedup already treats a reuse WriteConflict as "probe ruled out our prior attempt + seq genuinely contested" → clear pending, recompute from fresh meta. The probe-hit branch is unchanged: if our attempt actually committed at T1 the FSM no-ops before OCC, returning success regardless of what else happened.

Full case matrix:

• attempt 1 landed, no others → probe hit → no-op (return length)
• attempt 1 landed, others touched → probe hit → no-op (return length)
• attempt 1 missed, no others → probe miss + ReadKeys OK → apply
• attempt 1 missed, boundary touched → probe miss + ReadKeys conflict → recompute
• attempt 1 missed, same seq taken → probe miss + write OCC conflict → recompute

Regression test: TestListPushDedup_InterveningRPopRefusesStaleReuse reproduces codex's one-element [A] scenario and asserts the pushed element is reachable at meta.Head after the fix.

Lint / claude review minors:

• Deleted the dead onePhaseTxnRequest wrapper (golangci unused, also flagged by claude). Caller audit: zero remaining callers — all production routes through onePhaseTxnRequestWithPrevCommit since round-2.
• Refined optionalV2Fields comment to match how the table is actually consumed (decode iterates; encode mirrors via inline conditionals).

Caller audit (loop semantic-change rule): adding ReadKeys to the dedup-path dispatch only widens retry-on-conflict within retryRedisWrite; client-facing post-push length is unchanged; legacy gate-off path untouched.

Commit: 65daa3a2. All 5 dedup tests + kv/store suites green; gofmt + vet clean.

Claude finished @bootjp's task in 5m 12s —— View job

Round-3 Code Review

• Load tools
• Read Round-3 changed files: adapter/redis.go (boundary fence fix), kv/coordinator.go (dead wrapper deletion), kv/txn_codec.go (comment fix), adapter/redis_list_dedup_test.go (regression test)
• Analyze boundary-fence ReadKeys approach for correctness
• Check dead wrapper deletion and comment fix
• Post final review

Verdict: Round-3 changes are correct. The codex P1 gap is closed.

The boundary-fence fix, regression test, dead-code deletion, and comment correction are all sound. Two non-blocking observations follow.

Boundary-Fence Correctness (adapter/redis.go)

The mechanism is correct end-to-end:

Path when attempt 1 did NOT land and a pop/trim happened between attempts:

1. listPushBoundaryReadKeys captures listItemKey(Head) and (for non-empty list) listItemKey(Tail-1) at attempt-1 read time
2. These ride as OperationGroup.ReadKeys through pending.readKeys to the reuse dispatch
3. checkConflictsLocked (line store/mvcc_store.go:524) checks both write mutations and read keys against startTS; a tombstone from RPOP/LPOP at TS > startTS fires ErrWriteConflict
4. Adapter sees ErrWriteConflict, clears pending, recomputes from fresh meta ✓

Path when attempt 1 DID land:
The maybeProbe / FSM dedup probe fires before OCC and returns success — the boundary ReadKeys are never evaluated. Correct: subsequent pop/trim ops don't matter once our commit is confirmed at T1. ✓

Empty-list case (meta.Len ≤ 0 → readKeys = nil): No pops are possible on an empty list; concurrent RPUSH conflicts are caught by the write-key OCC on the item key itself. ✓

Dedup gate: listPushCore gates on r.onePhaseTxnDedup (line 3167) and routes accordingly; the legacy path is unchanged. ✓

Minor: Boundary fence is slightly over-conservative for RPUSH (non-blocking, safe)

listPushBoundaryReadKeys fences both Head and Tail-1 regardless of push direction. For RPUSH specifically, an LPOP that advances Head doesn't actually invalidate the reuse: RPUSH writes at old_Tail, and after LPOP the LRANGE window [new_Head, old_Tail+1) still includes old_Tail. Fencing Head for RPUSH will occasionally trigger a false ErrWriteConflict and recompute unnecessarily, but this never produces incorrect results — recomputation is the safe fallback. Likewise, RPOP doesn't invalidate LPUSH, but Tail-1 is fenced for LPUSH too.

This is the right tradeoff: the same listPushBoundaryReadKeys helper serves both RPUSH and LPUSH symmetrically with no direction-specific branching, and extra recomputes are cheap. Noting it only so a future reviewer doesn't mistake the false-positive OCC conflicts for a bug.

Regression Test (TestListPushDedup_InterveningRPopRefusesStaleReuse)

The test correctly simulates the P1 scenario:

1. Seeds [A] at seq 0 (Head=0, Len=1, Tail=1)
2. Dispatch 1: ambiguousDispatch=1, lands=false → returns ErrWriteConflict without applying. pending.readKeys = [listItemKey(key, 0)], pending.startTS = T_before_rpop
3. beforeDispatch(2): RPOP commits at TS > startTS — tombstones seq 0, writes delta {Head:+1, Len:-1}. State: Head=1, Len=0, Tail=1
4. Dispatch 2 (reuse): probe miss (attempt 1 didn't land) → occAdapterCoordinator calls ApplyMutations with ReadKeys=[listItemKey(key,0)], StartTS=T_before_rpop → checkConflictsLocked fires on the tombstoned key → ErrWriteConflict → pending = nil
5. Dispatch 3 (recompute): fresh meta (Head=1, Len=0), plans seq 1, empty ReadKeys (empty list). Succeeds. State: Head=1, Len=1, listItemKey(key,1)="v" ✓
6. Assertions: n==1, dispatches > 2 (= 3), meta.Head=1, st.GetAt(key, 1) == "v" ✓

Minor precision note (non-blocking): The assertion require.Greater(t, coord.dispatches, 2, ...) is intentionally loose. Given the fixed injection pattern (attempt 1 pre-rejects, attempt 2 OCC-conflicts, attempt 3 recomputes), == 3 would be tighter. > 2 is safe but won't catch a future regression where extra dispatches pile up before success. Low severity.

onePhaseTxnRequest Deletion (kv/coordinator.go)

Dead code correctly removed. All production paths route through onePhaseTxnRequestWithPrevCommit since round-2. golangci unused lint failure is resolved. ✓

optionalV2Fields Comment (kv/txn_codec.go)

The revised comment accurately describes the actual relationship: only the decoder iterates the table; the encoder uses matching inline conditionals. The "update both together" guidance is the actionable safeguard. ✓

Carry-over from Round-2 (unchanged, not in scope for Round-3)

For tracking — these remain open:

1. CommittedVersionAt retention guard (store/lsm_store.go:778, store/mvcc_store.go:476, codex P2): Both backends return (false, nil) when the probe timestamp is below minRetainedTS, making a compacted-history miss indistinguishable from "never landed." Should return ErrReadTSCompacted. Low urgency while retry windows are far shorter than compaction windows.

2. buildRedirectRequests unit coverage: No test calls this path with non-zero PrevCommitTS to verify the field survives the leader-redirect path. The code is correct (verified in round-2); the gap is test coverage only.

Summary: Round-3 is correct and complete for its stated scope. The two carry-over items (retention guard, redirect test) are the only open threads before merge.

@claude review

Round-4 addressing codex P1 + P2 posted at 06:51 (post-65daa3a2) plus your round-3 minor on the loose dispatch-count assertion.

codex P1 — pending saved even for non-retryable errors (adapter/redis.go:3339):
listPushCoreWithDedup recorded pending unconditionally on any first-attempt error, but retryRedisWrite only loops on store.ErrWriteConflict / kv.ErrTxnLocked, so for other errors the saved write set was discarded with the goroutine — misleading and useless.

Fix: only set/advance pending when isRetryableRedisTxnErr will actually trigger the loop. Both the fresh-attempt and reuse-retryable branches now gate the pending write/update. Visible behavior is unchanged in the non-retryable case (pending was always going to be lost); the code no longer pretends to hold state across a request boundary.

Scope-defining note: the broader window where an ambiguous error escapes retryRedisWrite and the client retries the whole command at a higher level (so the next listPushCoreWithDedup starts with no PrevCommitTS) requires a cross-request idempotency cache — out of scope for option 2. Will doc as a follow-up.

codex P2 — env flag advertised but never wired (adapter/redis.go:249):
ELASTICKV_REDIS_ONEPHASE_DEDUP was documented as the opt-in but NewRedisServer only read ELASTICKV_REDIS_TRACE. Added the env read in the struct literal next to it. Default off (!= "1"); WithOnePhaseTxnDedup option still overrides either way.

Round-3 minor (claude): tightened TestListPushDedup_InterveningRPopRefusesStaleReuse's dispatches > 2 to == 3 to pin the exact sequence (attempt 1 pre-rejects, reuse OCC-conflicts, recompute lands).

Caller audit (loop rule):

• pending-save gating is semantically a no-op: the unsaved state was always discarded by retryRedisWrite returning. No behavior change for non-retryable errors; no caller impact.
• env wiring expands the opt-in surface only through NewRedisServer (the production path). Tests that bypass it (literal &RedisServer{}) set onePhaseTxnDedup directly and are unaffected.

Commit: 7781725d. All 5 dedup tests green; fmt + vet clean.

Claude finished @bootjp's task in 3m 22s —— View job

Round-4 Code Review

• Load tools
• Read changed files: adapter/redis.go, adapter/redis_list_dedup_test.go
• Analyze pending-save gating (codex P1 fix)
• Analyze env wiring (codex P2 fix)
• Verify dispatch assertion tightening (round-3 minor)
• Check for remaining open items
• Post final review

Verdict: Round-4 changes are correct. Both targeted issues are resolved.

The pending-save gating and env wiring changes are sound. The dispatch count is correctly tightened. No new issues introduced.

P1 — Pending-save gating (adapter/redis.go)

Both guard sites are correct.

Fresh-attempt path (adapter/redis.go:3347):
pending = &reusableListPush{...} is now inside if isRetryableRedisTxnErr(dispErr). For non-retryable errors (context deadline, FSM apply failure, NOTLEADER), retryRedisWrite returns immediately after the closure exits — the closure's local pending is discarded regardless. Gating the assignment removes the misleading save-with-no-reader. ✓

For ErrWriteConflict and ErrTxnLocked, pending is still saved and retryRedisWrite loops. ✓

Reuse path (adapter/redis.go:3303):
pending.commitTS = commitTS is now inside if isRetryableRedisTxnErr(dispErr). The ErrWriteConflict early-return at line 3290-3296 already clears pending for that case before reaching line 3303, so the remaining retryable case reaching line 3303 is ErrTxnLocked (ambiguous — may have landed). Gating the update is correct: for non-retryable errors the commitTS update served no purpose since the loop won't fire. ✓

Minor (non-blocking): ErrWriteConflict on fresh attempt triggers an unnecessary probe.
When the fresh attempt returns ErrWriteConflict (definitive rejection — did NOT land), pending is saved and the next iteration takes the reuse path, which dispatches with PrevCommitTS = pending.commitTS. The FSM probes, gets a miss, then OCC on the stale boundary keys also fires (since the conflict implies an intervening write), clears pending, and recomputes. Compared to a fresh path that would go straight to recompute, this wastes one extra dispatch + probe per ErrWriteConflict on first attempt. Correct behavior, just slightly suboptimal. This is pre-existing behavior (not introduced by R4) and is bounded to the failure path, so low priority.

P2 — Env wiring (adapter/redis.go:502)

onePhaseTxnDedup: os.Getenv("ELASTICKV_REDIS_ONEPHASE_DEDUP") == "1",

Set in the struct literal before the option loop at line 515-519. WithOnePhaseTxnDedup(enabled) runs in that loop and correctly overrides the env value in either direction. Default off. ✓

The cmd/server/demo.go entrypoint creates the server via NewRedisServer (not a literal &RedisServer{}), so the env var now reaches production. ✓

Round-3 minor — dispatch count (adapter/redis_list_dedup_test.go:258)

require.Greater(t, coord.dispatches, 2, ...) → require.Equal(t, 3, coord.dispatches) pins the exact sequence (attempt 1 pre-rejects, attempt 2 reuse OCC-conflicts, attempt 3 recomputes and lands). Correct and tighter than the previous loose bound. ✓

Carry-over from prior rounds (not addressed in R4, both tracked in design doc)

1. CommittedVersionAt retention guard (store/lsm_store.go:778, store/mvcc_store.go:476): Both backends return (false, nil) when commitTS < minRetainedTS, making a compacted-history miss indistinguishable from "never landed." Should return ErrReadTSCompacted. Low urgency while compaction windows far exceed retry windows. [codex P2, also noted in Round-3]

2. buildRedirectRequests unit test: No test exercises the leader-redirect path with non-zero PrevCommitTS to verify it survives. Code is correct (verified in Round-2); gap is test coverage only.

Round-4 is clean. No further blocking issues. The two carry-over items are the only remaining open threads before merge.

@claude review

Round-8 addressing the new codex P1 at line 3306 (post-round-7): Preserve landed retry length on non-leader probes.

Round-7 returned (false, nil) unconditionally on a deposed leader, which let resolveReuseLength take the resolveListMeta fallthrough even when our prior had actually committed. The caller then returned the leader's current Len rather than pending.length, which under concurrent writes could include other clients' commits — breaking the per-our-commit linearizable Redis result codex flagged.

Fix: recover the fast path without a new RPC. On the non-leader path, submit a Raft ReadIndex via the existing LinearizableRead primitive (coordinator's on LeaderRoutedStore; per-group engine's on ShardStore). ReadIndex forwards to the current leader and waits until our local applied index reaches the leader's commit point — after that the local probe sees every committed version, so we can answer the exact-ts question authoritatively from the local replica. If ReadIndex fails (no leader reachable, ctx canceled), keep the (false, nil) fallback so the adapter still re-reads via the leader-fenced ScanAt/GetAt.

Cost: one ReadIndex round-trip on the deposed-node retry path (a rare situation). The leader-with-valid-lease fast path is unchanged.

Caller audit (loop rule):

• adapter/redis.go:3294 (resolveReuseLength): now sees authoritative true/false during churn, so the pending.length branch fires when our prior actually landed. The resolveListMeta fallthrough still triggers when the probe genuinely misses or ReadIndex fails.
• kv/fsm.go:505: routes through the raw store (not these wrappers), so the deterministic FSM apply path is untouched.

All 6 dedup tests + kv/store suites pass; gofmt + vet clean.

Commit: bc11779d.

Claude finished @bootjp's task in 3m 30s —— View job

Round-8 Code Review

• Load tools
• Read changed files: kv/leader_routed_store.go, kv/shard_store.go
• Analyze ReadIndex-fence approach for correctness
• Verify FSM path isolation
• Check carry-over items
• Post final review

Verdict: Round-8 changes are correct. The codex P1 "preserve landed retry length on non-leader probes" gap is closed on both wrappers.

The ReadIndex-fence approach is the right mechanism and the implementation is sound end-to-end. Three precision notes follow.

LeaderRoutedStore.CommittedVersionAt (kv/leader_routed_store.go:217)

if !s.leaderOKForKey(ctx, key) {
    if s.coordinator == nil { return false, nil }
    if _, err := s.coordinator.LinearizableRead(ctx); err != nil {
        return false, nil
    }
}
exists, err := s.local.CommittedVersionAt(ctx, key, commitTS)

Correctness of the ReadIndex fence:
Coordinate.LinearizableRead(ctx) → linearizableReadEngineCtx → engine.submitRead(ctx, true). With waitApplied=true, the engine submits a ReadIndex request to the Raft group: the current leader confirms its commit index and the follower waits until its local applied index reaches that point. Because:

• The reuse dispatch was already committed (it returned a result before resolveReuseLength is called)
• That commit is at some log index I_commit
• The leader's commit index when the ReadIndex is submitted is ≥ I_commit
• After LinearizableRead returns, the local applied index ≥ ReadIndex target ≥ I_commit
• s.local.CommittedVersionAt(ctx, key, commitTS) reads from the same local store that the FSM writes to

Therefore, after LinearizableRead returns successfully, the probe correctly answers "did our prior attempt land at T1?" ✓

leaderOKForKey semantics — exact-ts probe doesn't need timestamp advancement: leaderOKForKey discards the fence timestamp from leaderFenceTS. For exact-ts probes this is correct — we're asking "does version T1 exist?", not "read at max(ts, fenceTS)". Advancing the read timestamp would change the semantics from exact-version existence to snapshot visibility, defeating the dedup invariant. ✓

nil-coordinator fallback: s.coordinator == nil returns (false, nil) → resolveReuseLength falls through to resolveListMeta. Correct for test fixtures that don't wire a coordinator. ✓

ShardStore.CommittedVersionAt (kv/shard_store.go:118)

if !isLinearizableRaftLeader(ctx, engine) {
    if _, err := engine.LinearizableRead(ctx); err != nil {
        return false, nil
    }
}

No double ReadIndex on the non-leader path. isLinearizableRaftLeader calls isLeaderEngine(engine) first — engine.State() == raftengine.StateLeader. On a follower, isLeaderEngine returns false immediately and isLinearizableRaftLeader short-circuits without issuing a ReadIndex. Only then does CommittedVersionAt enter the non-leader branch and issue one ReadIndex via engine.LinearizableRead(ctx). The concern about double-charging the ReadIndex latency does not apply. ✓

Leader path cost: When we ARE the leader and the lease is valid, isLinearizableRaftLeader returns via leaseReadEngineCtx fast path (no ReadIndex), and we fall through directly to g.Store.CommittedVersionAt. This is identical to the pre-R8 behavior on the leader. ✓

Leader with expired lease: isLinearizableRaftLeader calls leaseReadEngineCtx → engine.LinearizableRead(ctx) → one ReadIndex. Returns true on success → reads local. One ReadIndex, same as all other GetAt/ScanAt callers. ✓

nil-engine guard (test fixtures): engine == nil → reads g.Store.CommittedVersionAt directly. Preserves existing test behavior where ShardStore is wired without Raft. ✓

FSM Path Isolation

kv/fsm.go:505 calls f.store.CommittedVersionAt where f.store is the per-shard raw store (e.g., *store.mvccStore or *store.pebbleStore) wired via NewKvFSMWithHLC(st, hlc). This is neither a LeaderRoutedStore nor a ShardStore — the R8 ReadIndex-fence methods don't affect it. The FSM apply path continues to read the local replica deterministically with no Raft round-trip. ✓

Minor: LeaderRoutedStore uses coordinator-level ReadIndex, not engine-level

s.coordinator.LinearizableRead(ctx) calls Coordinate.LinearizableRead(ctx) → linearizableReadEngineCtx(ctx, c.engine) → single-group engine. In a single-group deployment this is correct.

In a multi-group deployment, r.store would be a ShardStore (not LeaderRoutedStore), so this code path is single-group only. No issue — just noting the topology assumption. ✓

Open P1 from Round-6 (still unaddressed after Round-8)

The self-inflicted reuse conflict: when a reuse dispatch applies at commitTS2 but returns an ambiguous error before success is observed, the next retry (with PrevCommitTS=T1) sees ErrWriteConflict from its own committed reuse write — dispatchListPushReuse treats any reuse WriteConflict as "genuine third-party conflict," clears pending, and recomputes, potentially appending a duplicate. R8 does not address this; it's the last open correctness thread before merge.

Track this →

Carry-over from Prior Rounds (unchanged)

1. CommittedVersionAt retention guard (store/lsm_store.go:778, store/mvcc_store.go:476): Both backends return (false, nil) when commitTS < minRetainedTS instead of ErrReadTSCompacted. In the degraded case (compaction window shorter than retry window), resolveReuseLength safely falls through to resolveListMeta. Low urgency.

2. buildRedirectRequests unit test: No test verifies PrevCommitTS survives the leader-redirect path with a non-zero value. Code correct (round-2); gap is test coverage only.

Summary: Round-8 is correct and complete for its stated scope. The ReadIndex-fence correctly turns the non-leader probe from "unconditional false" into "wait-and-read-local," preserving pending.length when our prior attempt actually committed. The open P1 from Round-6 (self-inflicted reuse conflict) and the two carry-over items remain the only threads before merge.

@claude review

Round-9 fixing the two nilerr lint violations reviewdog/golangci flagged on round-8's LinearizableRead fence:

kv/leader_routed_store.go:226 error is not nil (line 225) but it returns nil
kv/shard_store.go:143         error is not nil (line 142) but it returns nil

The nilerr signature is if err != nil { return nil }. My intent — treating ReadIndex failure as "couldn't verify, fall back to the leader-routed slow path" rather than as a real error to surface to the caller — matches LatestCommitTS's contract but the linter has no way to see that.

Fix: extract small helpers (LeaderRoutedStore.tryLinearizableFence and package-level tryEngineLinearizableFence) that report ReadIndex success as a bool, then express the call site as:

if !leaderOK && !fenceOK {
    return false, nil
}

Same semantics, no error swallowed at a return statement, no //nolint comment.

All 6 dedup tests pass; gofmt + vet clean.

Commit: b4695108.

Claude finished @bootjp's task in 2m 49s —— View job

Round-9 Code Review

• Load tools
• Read changed files: kv/leader_routed_store.go, kv/shard_store.go
• Analyze tryLinearizableFence / tryEngineLinearizableFence helper extraction
• Verify lint fix and semantic equivalence
• Check carry-over items from prior rounds
• Post final review

Verdict: Round-9 changes are correct. Both nilerr violations are resolved without behavior change.

The helper extraction correctly converts the "error-treated-as-bool" pattern into a proper bool-returning API. Semantic equivalence with R8 is verified. One minor observation about the helpers' relationship to each other follows.

Nilerr Fix — LeaderRoutedStore.CommittedVersionAt (kv/leader_routed_store.go:221)

Old R8 pattern (triggered nilerr):

if !s.leaderOKForKey(ctx, key) {
    if s.coordinator == nil { return false, nil }
    if _, err := s.coordinator.LinearizableRead(ctx); err != nil {
        return false, nil  // ← nilerr: err ≠ nil but return nil error
    }
}

New R9 pattern (clean):

if !s.leaderOKForKey(ctx, key) && !s.tryLinearizableFence(ctx) {
    return false, nil  // ← no error in scope; clean
}

Semantic equivalence confirmed:

The coordinator-nil case previously had an explicit early return (if s.coordinator == nil { return false, nil }). In R9, tryLinearizableFence guards s.coordinator == nil → return false → !fenceOK = true → combined condition true → same result. ✓

Nilerr Fix — ShardStore.CommittedVersionAt (kv/shard_store.go:133)

Old R8 pattern (triggered nilerr):

if !isLinearizableRaftLeader(ctx, engine) {
    if _, err := engine.LinearizableRead(ctx); err != nil {
        return false, nil  // ← nilerr
    }
}

New R9 pattern (clean):

if !isLinearizableRaftLeader(ctx, engine) && !tryEngineLinearizableFence(ctx, engine) {
    return false, nil
}

Semantically identical under the same case analysis. The isLinearizableRaftLeader short-circuit means no tryEngineLinearizableFence is called when the leader check passes, preserving the R8 cost profile. ✓

Note (non-blocking, pre-existing from R8): For the "leader with expired lease AND first ReadIndex fails" case, isLinearizableRaftLeader internally calls engine.LinearizableRead and fails, then tryEngineLinearizableFence tries a second ReadIndex. This double-ReadIndex was present in R8 and is preserved in R9 — not introduced by the refactor. It's bounded to a narrow failure window (expired lease + unreachable leader).

Helper Design

tryLinearizableFence (method on LeaderRoutedStore) vs tryEngineLinearizableFence (package-level function): The asymmetry is intentional and correct. LeaderRoutedStore holds the Coordinate interface, while ShardStore accesses the raftengine.LeaderView directly. The method form allows the s.coordinator == nil guard to be expressed naturally inside the helper; the package-level form takes the engine as a parameter and guards engine == nil.

Both helpers have clear comment blocks explaining the intentional error-suppression design — this is the key requirement for avoiding a //nolint directive (the intent is readable, the type system expresses it). ✓

Codex P2 from R8 — kv/sharded_coordinator.go:393 (new, not addressed in R9)

The latest codex review (on bc11779) flagged that multi-shard retries with a non-zero PrevCommitTS fall through to the 2PC path (prewriteTxn/commitPrimaryTxn), which doesn't encode PrevCommitTS in the log entries. The dedup probe never reaches the FSM for multi-shard retries.

In practice, the list-push dedup feature operates only on single-key (single-shard) RPUSH/LPUSH commands, so this gap doesn't affect the primary use case. Codex's suggestion — fail multi-shard retries with non-zero PrevCommitTS explicitly rather than silently honoring them without dedup semantics — is a reasonable defensive measure. Worth tracking for a follow-up but not blocking merge.

Carry-over from Prior Rounds (unchanged)

1. Open P1 from Round-6 (adapter/redis.go, dispatchListPushReuse): self-inflicted reuse conflict — reuse dispatch applies at commitTS2 but returns ambiguous error; next retry sees ErrWriteConflict from its own committed write, clears pending, and potentially appends a duplicate. The fix is to probe CommittedVersionAt(firstWriteKey(pending.ops), pending.commitTS) before treating a reuse-path WriteConflict as a genuine third-party conflict.

2. CommittedVersionAt retention guard (store/lsm_store.go:778, store/mvcc_store.go:476): Both backends return (false, nil) when commitTS < minRetainedTS instead of ErrReadTSCompacted. In the degraded case, resolveReuseLength gracefully falls through to resolveListMeta. Low urgency while compaction windows far exceed retry windows.

3. buildRedirectRequests unit test: No test verifies PrevCommitTS survives the leader-redirect path with a non-zero value. Code is correct (verified round-2); gap is test coverage only.

Summary: Round-9 is a correct, minimal lint fix. The nilerr violations are resolved by converting the error-used-as-bool pattern into properly-typed bool-returning helpers with clear documentation. No behavior change. The open P1 (self-inflicted reuse conflict), the new codex P2 (multi-shard PrevCommitTS silent drop), and the two carry-over items remain the only open threads before merge.

@claude review

Round 10: addressed all 5 outstanding inline findings (commit 71430f2).

P1

1. Self-inflicted reuse conflict (adapter/redis.go dispatchListPushReuse) — on ErrWriteConflict after reuse, probe CommittedVersionAt(probeKey, commitTS); if our own reused commit landed, return success instead of bubbling the conflict. Regression test TestListPushDedup_SelfInflictedReuseConflict_ReturnsSuccess added.
2. Linchpin doc (docs/design/2026_05_21_proposed_txn_secondary_idempotency.md) — re-proved race-freedom under cancellable caller ctx. Ordering derives from FIFO proposeCh + monotonic raft log indices and fresh-proposal-on-new-leader semantics, not from context.Background() blocking (which was the stale argument).

P2
3. Retention guard (store/{lsm_store,mvcc_store}.go CommittedVersionAt) — return ErrReadTSCompacted when probed commit_ts is below the retention watermark; callers fall back to safe path. Uniform with all other ErrReadTSCompacted consumers in tree.
4. Multi-shard PrevCommitTS reject (kv/sharded_coordinator.go dispatchTxn) — fail-closed ErrTxnDedupRequiresSingleShard when an incoming txn request carries PrevCommitTS but spans shards. The 2PC log builders encode only CommitTS, so silently honoring such a request would drop the probe at the FSM and let :duplicate-elements reappear.
5. Avoid failing after reuse apply (adapter/redis.go resolveReuseLength) — on reuse, if the post-commit meta-read fails, fall back to pending.length; the apply has already landed, refusing the client response is strictly worse than reporting the locally-known length.

Caller audit (per the /loop semantic-change rule):

• CommittedVersionAt: 5 call sites; all treat (false, ErrReadTSCompacted) uniformly as "couldn't probe, fall back to safe path".
• ErrTxnDedupRequiresSingleShard: single emit site, surfaced to client as a commit error — fail-closed by design.
• ErrReadTSCompacted: existing callers (grpc adapter, dualwrite, s3 adapter, kv compactor) already treat as soft signal; new sites consistent.

Tests: go test ./adapter/ -run ListPushDedup 7/7 pass (incl. new P1 regression); go test ./kv/ ./store/ both pass; gofmt + go vet clean.

TLA+ spec divergence review (auto-triggered)

This PR touches files that the TLA+ safety spec has an anchor on (per
docs/design/2026_05_28_partial_tla_safety_spec.md §3),
so an AI review is requested below to verify the implementation has not drifted
from the model.

Anchored files changed in this PR head (71430f2):

• kv/coordinator.go
• kv/fsm.go
• kv/sharded_coordinator.go
• store/mvcc_store.go

What to check, by subsystem:

• kv/hlc*.go — Next() must respect the HLC-4 preconditions (i)/(ii)/(iii) from the design doc: bounded skew, logical-counter handoff on leader change (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). Any change to renewal cadence, group selection, or fail-closed behaviour is in scope.
• kv/transaction.go, kv/lock_resolver.go — OCC commit-ts assignment, lock-map encoding (key, lock_ts) -> start_ts, and the LockResolver action OCC-3 depends on. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• distribution/** — route catalog versioning, SplitRange atomicity, and CatalogWatcher async fan-out (M4 scope).

If the change is correct but requires a spec update, edit tla/hlc/HLC.tla (or the corresponding M2..M5 module once landed) and the design doc in the same PR. The tla-check workflow runs the TLC model check on the same paths.

@claude review please verify TLA+ spec divergence per the checklist above.

@codex review please verify TLA+ spec divergence per the checklist above.